You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The PR adds input validation but doesn't implement the main requirement of using the dedicated endpoint for getting named cookies. The code still fetches all cookies and filters them.
Use more precise string validation to allow whitespace-containing cookie names
Consider using string.IsNullOrEmpty() instead of IsNullOrWhiteSpace() for cookie name validation, as whitespace-only cookie names might be valid in some scenarios.
-if (string.IsNullOrWhiteSpace(name))+if (string.IsNullOrEmpty(name))
{
- throw new ArgumentException("Cookie name cannot be empty", nameof(name));+ throw new ArgumentException("Cookie name cannot be null or empty", nameof(name));
}
Apply this suggestion
Suggestion importance[1-10]: 7
Why: Using IsNullOrEmpty() instead of IsNullOrWhiteSpace() is more appropriate for cookie name validation since whitespace-only names could be valid. This improves correctness of the validation logic.
7
✅ Enhance error messages with actual values for better debugging experienceSuggestion Impact:The commit modified the error message for empty cookie name, though in a different way than suggested - by clarifying that the name cannot be null or empty instead of showing the actual value
code diff:
- throw new ArgumentException("Cookie name cannot be empty", nameof(name));+ throw new ArgumentException("Cookie name cannot be null or empty", nameof(name));
Include the actual value in the exception message to help with debugging when an invalid cookie name is provided.
-throw new ArgumentException("Cookie name cannot be empty", nameof(name));+throw new ArgumentException($"Cookie name cannot be empty. Provided value: '{name}'", nameof(name));
Apply this suggestion
Suggestion importance[1-10]: 4
Why: Including the actual invalid value in the exception message would help with debugging, though the current message is already clear. This is a minor improvement to developer experience.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Description
Empty cookie name should be invalid when deleting.
Motivation and Context
Contributes to #15044
Types of changes
Checklist
PR Type
Enhancement, Bug fix
Description
Added validation for empty or null cookie names in
DeleteCookieNamedandGetCookieNamedmethods.Updated exception type to
ArgumentExceptionfor invalid cookie names.Simplified dictionary initialization in
DeleteCookieNamedmethod.Improved code consistency and error handling in cookie management methods.
Changes walkthrough 📝
CookieJar.cs
Enhance cookie management with validation and exceptionsdotnet/src/webdriver/CookieJar.cs
ArgumentExceptionfor invalid names.DeleteCookieNamed.GetCookieNamedmethod.ICookieJar.cs
Update cookie interface documentation and validationdotnet/src/webdriver/ICookieJar.cs
DeleteCookieNameddocumentation.